Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for destructured records #7

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add support for destructured records #7

wants to merge 4 commits into from

Conversation

rfvgyhn
Copy link

@rfvgyhn rfvgyhn commented Dec 7, 2023

This PR adds support for patterns that destructure a record with positional params.

abstract record Root
{
    Root() {}
    public sealed record Leaf1(int Value) : Root {}
    public sealed record Leaf2 : Root {}
}


return root switch
{
    Root.Leaf1(var v) => 0,
    Root.Leaf2 => 1,
};

I wasn't sure how to create a unit test with a specific editor config so I added a new constructor param to the suppressors. Let me know if that's no good.

@rfvgyhn rfvgyhn changed the title Add support for destructure records Add support for destructured records Dec 7, 2023
@shuebner
Copy link
Owner

I have seen the PR but have not had time yet to look at it, sorry.

shuebner
shuebner requested changes Feb 3, 2024
View reviewed changes
Copy link
Owner

@shuebner shuebner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice PR, thank you for understanding and honoring the current project structure

ClosedTypeHierarchyDiagnosticSuppressor/SwitchExpressionSuppressor.cs Outdated Show resolved Hide resolved
@rutgersc
Copy link

rutgersc commented Jun 5, 2024

Ran into this as well, would be nice to have this in a new version.

shuebner
shuebner requested changes Jun 9, 2024
View reviewed changes
Copy link
Owner

@shuebner shuebner left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except missing some negative tests accompanying the already added positive tests for destructuring, this looks good.

ClosedTypeHierarchyDiagnosticSuppressor.Tests/SwitchStatementSuppressorTests.cs Show resolved Hide resolved
shuebner
shuebner requested changes Jun 22, 2024
View reviewed changes
ClosedTypeHierarchyDiagnosticSuppressor.Tests/SwitchExpressionSuppressorTests.cs
@@ -107,6 +107,26 @@ public static int DoSwitch(Root root)

return EnsureSuppressed(code, NullableContextOptions.Enable, allowRecords: true);
}

[Test]
public Task When_record_with_destructure_And_only_base_type_is_matched_Then_do_not_suppress()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of the tests are the wrong way around. The test should define a hierarchy where Leaf1 has a object v and then test that a match agains Leaf1(string v) is not exhaustive.

The fact that the current wrong tests succeed is due to a missing feature implementation in the IsDeclarationPatternNonRestrictive method. The method currently expects containingSubpatternSyntax.NameColon to be present, whereas with deconstruction it is not.

Please fix the tests and add the implementation accordingly.

The positive tests (that assert suppression) happen to succeed correctly because they use var. Had you written them with object v, they would have failed as well and revealed the missing implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shuebner shuebner shuebner requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rfvgyhn @shuebner @rutgersc